Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make AbstractFastaSequenceFile serializable by Kryo for Spark. #1408

Merged
merged 1 commit into from
Aug 13, 2019
Merged

Make AbstractFastaSequenceFile serializable by Kryo for Spark. #1408

merged 1 commit into from
Aug 13, 2019

Conversation

tomwhite
Copy link
Contributor

See explanation at EsotericSoftware/kryo#469

This change is a fix for broadinstitute/gatk#6091, which is a regression in GATK running on Spark. I've tested the fix manually with GATK.

/cc @lbergelson

@codecov-io
Copy link

codecov-io commented Aug 13, 2019 •
edited
Loading

Codecov Report

Merging #1408 into master will decrease coverage by 0.015%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #1408       +/-   ##
===============================================
- Coverage     68.101%   68.086%   -0.015%     
+ Complexity      8373      8371        -2     
===============================================
  Files            573       573               
  Lines          33963     33963               
  Branches        5668      5668               
===============================================
- Hits           23129     23124        -5     
- Misses          8645      8648        +3     
- Partials        2189      2191        +2
Impacted Files Coverage Δ Complexity Δ
.../samtools/reference/AbstractFastaSequenceFile.java 57.5% <100%> (ø) 12 <2> (ø) ⬇️
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java 78.947% <0%> (-10.526%) 3% <0%> (-1%)
...samtools/util/AsyncBlockCompressedInputStream.java 72% <0%> (-4%) 12% <0%> (-1%)

@lbergelson
Copy link
Member

@tomwhite Is this happening now because of the kryo update or because of htsjdk changes?

lbergelson
lbergelson approved these changes Aug 13, 2019
View reviewed changes
Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, thank you.

@lbergelson lbergelson merged commit 9401637 into samtools:master Aug 13, 2019
@tomwhite
Copy link
Contributor Author

@lbergelson htsjdk certainly has a change in the last release that caused this. Whether going from Kryo 3 to 4 also made a difference, I'm not sure. (Thanks for merging.)

@tomwhite tomwhite mentioned this pull request Sep 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbergelson lbergelson lbergelson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomwhite @codecov-io @lbergelson